[TRTLLMINF-188][infra] Require approval for broad post-merge bot runs#16318
Conversation
📝 WalkthroughWalkthroughChangesPost-merge approval enforcement
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant PullRequest
participant ApprovalWorkflow
participant TeamsAPI
participant GitHubAPI
PullRequest->>ApprovalWorkflow: Apply approval label
ApprovalWorkflow->>GitHubAPI: Find latest label event
ApprovalWorkflow->>TeamsAPI: Verify labeling actor
TeamsAPI-->>ApprovalWorkflow: Return membership status
ApprovalWorkflow->>GitHubAPI: Remove invalid label and post comment
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/scripts/test_post_merge_approval_workflow.js:
- Around line 87-129: Update the runValidation test fixture so context.actor and
payload.sender.login use distinct values when sender is present, then assert the
result uses the sender login (for example, retain the expected label actor while
using a different fallback actor). Preserve the sender=false case to verify
context.actor is used when no sender exists.
In @.github/workflows/post-merge-approval.yml:
- Around line 70-103: Update the post-merge approval workflow to serialize or
debounce runs per pull request, then re-check the latest label event and
authorization state immediately before any removeLabel cleanup. Ensure an older
unauthorized run exits without removing a label re-added by a later valid
approval.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 43c183c7-c921-4987-97dc-214454fce7d4
📒 Files selected for processing (4)
.github/scripts/test_post_merge_approval_workflow.js.github/workflows/bot-command.yml.github/workflows/post-merge-approval.ymldocs/source/developer-guide/ci-overview.md
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/source/developer-guide/ci-overview.md (1)
1-1: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd the required NVIDIA copyright header.
This modified Markdown file starts directly with the document title and has no NVIDIA copyright header. Add the repository-standard header in a Markdown-safe comment and use the latest meaningful modification year.
As per coding guidelines, all files matching
**/*must contain or update the NVIDIA copyright header.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/source/developer-guide/ci-overview.md` at line 1, Add the repository-standard NVIDIA copyright header at the beginning of the Markdown document using a Markdown-safe comment, and set its year to the latest meaningful modification year. Keep the existing “Continuous Integration Overview” title and document content unchanged.Source: Coding guidelines
🧹 Nitpick comments (1)
.github/workflows/post-merge-approval.yml (1)
18-20: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low value
pull_request_targetdangerous-trigger finding is an expected trade-off here, not a defect.zizmor's
dangerous-triggersrule fires on anypull_request_targetuse, but this workflow never checks out or executes PR code — the script body is static and only reads timeline/label metadata via the API, so the usual fork-RCE/secret-exfiltration vector doesn't apply. Consider documenting this rationale near the trigger (or adding a suppression entry in zizmor's config, if supported) so the finding doesn't need to be re-litigated on every scan.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/post-merge-approval.yml around lines 18 - 20, Document the intentional use of pull_request_target beside the trigger in the workflow, noting that the job runs only static logic and reads timeline/label metadata through the API without checking out or executing pull-request code. Alternatively, add the supported zizmor suppression for this expected finding, preserving the existing trigger behavior.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/post-merge-approval.yml:
- Around line 22-24: Add issues: write to the permissions block in the
post-merge approval workflow, preserving the existing contents and pull-requests
permissions so github.rest.issues.removeLabel can execute successfully.
---
Outside diff comments:
In `@docs/source/developer-guide/ci-overview.md`:
- Line 1: Add the repository-standard NVIDIA copyright header at the beginning
of the Markdown document using a Markdown-safe comment, and set its year to the
latest meaningful modification year. Keep the existing “Continuous Integration
Overview” title and document content unchanged.
---
Nitpick comments:
In @.github/workflows/post-merge-approval.yml:
- Around line 18-20: Document the intentional use of pull_request_target beside
the trigger in the workflow, noting that the job runs only static logic and
reads timeline/label metadata through the API without checking out or executing
pull-request code. Alternatively, add the supported zizmor suppression for this
expected finding, preserving the existing trigger behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: f8a7cfaf-7771-40f5-8315-71b47658331e
📒 Files selected for processing (4)
.github/scripts/test_post_merge_approval_workflow.js.github/workflows/bot-command.yml.github/workflows/post-merge-approval.ymldocs/source/developer-guide/ci-overview.md
|
Addressed the latest CodeRabbit review on head
Validation: executable workflow contract test passed, |
ZhanruiSunCh
left a comment
ZhanruiSunCh
left a comment
There was a problem hiding this comment.
One concern regarding the interaction between this workflow and the Jenkins gate:
Potential bypass via workflow cancellation / delay:
The clear step condition if: always() && \!cancelled() && ... means the label cleanup is skipped when the workflow run is cancelled. A user with Write access (but not a trt-llm-ci-approvers member) could:
- Add the
ci: post-merge approvedlabel - Cancel the approval workflow run before the clear step executes
- The label remains on the PR
/bot run --post-merge→ Jenkins sees the label → dispatches
The same gap exists when the workflow is queued (concurrency group) or delayed (GitHub Actions runner unavailability) — the label is unvalidated during that window.
Since the Jenkins wrapper job already has a GitHub token, consider adding the same membership verification in validatePostMergeApproval() (fetch PR timeline → check label actor membership in trt-llm-ci-approvers) so Jenkins can independently validate regardless of the workflow's execution state.
Signed-off-by: Yanchao Lu <yanchaol@nvidia.com>
Signed-off-by: Yanchao Lu <yanchaol@nvidia.com>
Signed-off-by: Yanchao Lu <yanchaol@nvidia.com>
Signed-off-by: Yanchao Lu <yanchaol@nvidia.com>
Signed-off-by: Yanchao Lu <yanchaol@nvidia.com>
Signed-off-by: Yanchao Lu <yanchaol@nvidia.com>
Signed-off-by: Yanchao Lu <yanchaol@nvidia.com>
b9e1420 to
ebde870
Compare
|
@ZhanruiSunCh Thanks for raising the cancellation/delay concern in your review. I agree that making this a strict authorization boundary would require Jenkins to synchronously verify the label actor/Team membership (or consume another trusted approval record). For TRTLLMINF-188 we are intentionally keeping the lower-complexity, best-effort resource-governance model: the GitHub Action validates the actor, and the companion Jenkins change refreshes current label presence before dispatch. This update covers the normal cancellation path after a Guard job starts:
Queued-before-start cancellation, force-cancel, runner/service failure, and the immediate asynchronous label-to-dispatch window are not fully covered by this design. Follow-up |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
|
/bot skip --comment "Not tested by L0" |
|
PR_Github #60159 [ skip ] triggered by Bot. Commit: |
|
PR_Github #60159 [ skip ] completed with state |
3 participants
Description
TRTLLMINF-188 reduces accidental consumption of shared CI resources by requiring the
ci: post-merge approvedlabel before broad post-merge bot runs are dispatched.This PR:
Guard Post-Merge Approval Labelworkflow.NVIDIA/trt-llm-ci-approvers.pull_request_targetonly for static default-branch API logic and never checks out or executes PR-controlled code./bothelp and CI documentation while preserving explicit stage names and limited wildcard behavior such as*PerfSanity*.The companion Jenkins change in
trtllm-github-ci!86refreshes current PR labels during command validation and immediately before downstream dispatch. It requires the approval label for:--post-merge--stage-list "*"and--extra-stage "*"--stage-list "*Post-Merge*"and--extra-stage "*Post-Merge*"*, or placing them in comma-separated selector listsSelector matching remains case-sensitive, consistent with downstream stage selection. Approval-label identity is handled case-insensitively by both the workflow and Jenkins.
The membership lookup uses
TRTLLM_AGENT_SHARED_TOKEN; label cleanup and PR comments use the scopedGITHUB_TOKEN.Scope and limitations
This is a best-effort CI resource-governance guard, not a strict authorization boundary. Jenkins checks current label presence but does not synchronously revalidate the label actor or Team membership. A Guard job that has started can attempt cleanup after standard cancellation, but queued-before-start cancellation, force-cancel, runner/service failure, and the immediate asynchronous label-to-dispatch window are not fully covered. TRTLLMINF-220 tracks the stronger guarantee and follow-up design.
The canonical
.github/workflows/blossom-ci.ymlhas no diff in this PR.Validation
Source and isolated Jenkins checks
post-merge-approval.ymlSHA-256:7e67834e0ede75ce84e74bf78a5d4a5fdc744b609fc6ae10b1eeb29b00f51cf5.github-scriptcompilation, focused case-insensitive label validation/cleanup simulation, andgit diff --checkpassed.08886d03b7f49c6b86982ff134559e8f323c0be9224850298345c897e8f21905.Personal-fork E2E
The isolated tests used personal-fork PR 11, which is closed, draft, unmerged, marker-only, and label-clean.
The authorized-member, commit-persistence, and non-member scenarios used the current production workflow with only the repository predicate adapted to
chzblych/TensorRT-LLM:synchronize-triggered Guard run appeared.sha256:56ff6931ea189a89d22c743938a1ce5cc2024a5cd6541509d4d937c5ca6f92fd.For the standard-cancellation scenario, the fork workflow used the same implementation plus a temporary deterministic pause step solely to create a reproducible cancellation window:
SHA256SUMS; digest:sha256:0210abec37b9311998af6979380e2ec97edc6a5a9767e5a8774c8b71425631f8.Earlier deterministic E2E remains applicable to the unchanged event-binding and concurrency logic:
After testing, the temporary Guard and deterministic pause were removed from fork
main, and the retained negative-E2E driver was restored. These personal-fork tests invoked no/botcommand, Blossomissue_commentworkflow, or production downstream Jenkins job.The upstream PR was explicitly marked as not requiring L0 with
/bot skip; no L0/downstream test workload was dispatched.Deployment and Merge Order
main, then re-run required checks and automated review.TRTLLM_AGENT_SHARED_TOKENwith organization/Team-membership read access.ci: post-merge approvedafter the Guard is active and before enabling the Jenkins gate.trtllm-github-ci!86.Deploying the actor-validation workflow first avoids a window in which Jenkins trusts a label before unauthorized additions are being cleaned up.
PR Checklist
Please review the following before submitting your PR:
PR description clearly explains what and why. If using CodeRabbit's summary, please make sure it makes sense.
PR Follows TRT-LLM CODING GUIDELINES to the best of your knowledge.
Test cases are provided for new code paths (see test instructions)
If PR introduces API changes, an appropriate PR label is added - either
api-compatibleorapi-breaking. Forapi-breaking, includeBREAKINGin the PR title.Any new dependencies have been scanned for license and vulnerabilities
CODEOWNERS updated if ownership changes
Documentation updated as needed
Update tava architecture diagram if there is a significant design change in PR.
The reviewers assigned automatically/manually are appropriate for the PR.
Please check this after reviewing the above items as appropriate for this PR.
GitHub Bot Help
To see a list of available CI bot commands, please comment
/bot help.